Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

metal: Set preserveInvariance for shader options #2372

Merged
merged 1 commit into from
Jan 12, 2022
Merged

metal: Set preserveInvariance for shader options #2372

merged 1 commit into from
Jan 12, 2022

Conversation

scoopr
Copy link
Contributor

@scoopr scoopr commented Jan 9, 2022

This fixes, for example, depth prepass resulting in ever so slightly
different depth values for later render passes.

Connections
BVE-Reborn/rend3#317

Description
By default, metal enables "fastMath" optimizations for shaders, which may result in ever so slightly values for, for example depth, or other values. Setting the preserveInvariance pessimizes this for vertex shader locations, so that depth prepasses work consistently.

Testing
With rend3 examples

@scoopr
Copy link
Contributor Author

scoopr commented Jan 9, 2022

Not sure what the webgpu spec says about these accuracy issues, but I'm guessing that disabling fastMath itself shouldn't be needed, but preserveInvariance makes sense at least as a default, as it is a portability issue.

@cwfitzgerald cwfitzgerald added the PR: needs back-porting PR with a fix that needs to land on crates label Jan 10, 2022
@cwfitzgerald cwfitzgerald requested a review from kvark January 10, 2022 00:26
kvark
kvark reviewed Jan 10, 2022
View reviewed changes
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First the user code is expected to put [[invariant]] on the position output for this to be portable.
Honestly, my preference was to just force it unconditionally, but the group wanted it to be optional.
Then, given [[invariant]] the implementations are expected to do the thing, but MSL doesn't universally support it, so it becomes an unofficial best-effort scenario.

That is to say, I'm fully on board with calling set_preserve_invariance whenever it's available.

wgpu-hal/src/metal/device.rs Outdated Show resolved Hide resolved
@scoopr
Copy link
Contributor Author

scoopr commented Jan 10, 2022

Ah, so the "right" way to do it, is to add [[invariant]] to the position in the wgsl, and that should percolate to [[invariant]] in msl, invariant in glsl, the decorator in spirv, and I guess precise in hlsl?

But I suppose it is not currently doing that in naga?

@kvark
Copy link
Member

kvark commented Jan 10, 2022

Right, unfortunately. Filed gfx-rs/naga#1659 to follow-up.

@scoopr scoopr force-pushed the fix-metal-invariance branch from f364de1 to e052572 Compare January 10, 2022 22:15
@scoopr
Copy link
Contributor Author

scoopr commented Jan 10, 2022

I actually thought the [[invariant]] would replace the need for the preserveInvariance setting, but I just now took another look at the msl spec

This calculation is now
guaranteed to be invariant when passing -fpreserve-invariance option or setting the
preserveInvariance on the MTLCompilerOptions from the Metal API for runtime
compilation. Note that [[invariant]] is ignored if the options are not passed.

So, it needs both the flag and the decoration? But just the option still seems to fix the issues? I don't get it?

Also for the older versions, it says

Compilers prior to IOS 14.0 and macOS 11.0,
the calculation is likely (although not guaranteed) to be invariant.

@kvark
Copy link
Member

kvark commented Jan 11, 2022

Compilers prior to IOS 14.0 and macOS 11.0,
the calculation is likely (although not guaranteed) to be invariant.

Oh fun! I wonder if it's a new edit.

Anyway, looks like a mess, but we should proceed with the same plan. I.e. we'll always set preserveInvariance on the host side, and shaders may or may not have [[invariant]] spewed in them.

@scoopr
metal: Set preserveInvariance for shader options
7543c2e 
This enables invariance for metal, for stable vertex positions, which
are useful for depth prepass and similiar things.
@scoopr scoopr force-pushed the fix-metal-invariance branch from e052572 to 7543c2e Compare January 11, 2022 21:29
@scoopr
Copy link
Contributor Author

scoopr commented Jan 11, 2022

Cleaned out the possibly misleading comment. Ok now?

kvark
kvark approved these changes Jan 12, 2022
View reviewed changes
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kvark kvark merged commit 99b3a6e into gfx-rs:master Jan 12, 2022
@scoopr scoopr deleted the fix-metal-invariance branch January 13, 2022 19:25
@kvark kvark mentioned this pull request Jan 21, 2022
Merged
@kvark
Copy link
Member

kvark commented Jan 21, 2022

published in wgpu-hal-0.12.3

@kvark kvark removed the PR: needs back-porting PR with a fix that needs to land on crates label Jan 21, 2022
@cwfitzgerald cwfitzgerald mentioned this pull request Jan 23, 2022
Closed
@francesca64 francesca64 mentioned this pull request Feb 4, 2022
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark kvark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scoopr @kvark @cwfitzgerald